-
-
Notifications
You must be signed in to change notification settings - Fork 169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve handling empty segments in urls according to RFC3986 #1026
Improve handling empty segments in urls according to RFC3986 #1026
Conversation
7d119c4
to
5ed39cb
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1026 +/- ##
==========================================
+ Coverage 61.96% 62.06% +0.09%
==========================================
Files 38 38
Lines 6510 6550 +40
Branches 353 352 -1
==========================================
+ Hits 4034 4065 +31
- Misses 2448 2457 +9
Partials 28 28
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@Dreamsorcerer can you help me figure out the issue with the codecov/ checks? I can see it ran the unit tests, but failed to create proper coverage files? |
I'd change the fail option to true, then it'll be obvious if any uploads have failed (and which jobs need to be rerun): |
It's not an upload failing. Yet the only tests which are run evaluated are the tests which have a annotation to return None You can see test_url.py and tests/test_url.py exist. Same issue: https://app.codecov.io/gh/aio-libs/yarl/pull/1025/tree Same problem: https://app.codecov.io/gh/aio-libs/yarl/pull/1015/blob/tests/test_url.py |
Oh, that's because webknjaz added mypy coverage to the results. I think that's just confusing... If you'd like to improve the situation, then you can add tests/ to coverage in pytest.ini and look at enabling the options we use elsewhere in .mypy.ini while fixing up the errors it highlights (maybe add a couple of options in each PR). You can use aiohttp-jinja2 as a reference: https://github.com/aio-libs/aiohttp-jinja2 |
MyPy has it's own flag, the flag I chose here is Py-3.12.4, still showing the mypy results? May be better to have somebody look into this who has an idea how this is supposed to work. |
Yeah, but it's clearly mypy coverage. See above if you want to improve either coverage. |
To my understanding … The coverage reported by mypy is "tests/test_url.py", the coverage reported by python 3.12.4 is for "test_url.py". The reported codecov numbers are combined values for unit testing and type checking. Due to a large number of single file posts, codecov may rate limit (seen in the scheduled CI runs), I guess this could be prevented by limiting the number of requests, combining all reports in a single upload. |
Oh, wait, I see what you mean. Codecov wouldn't load anything last time I looked. So, it looks like changing from codecov v3 to v4 resulted in the file paths changing for some reason... |
Well, copying the config from aiohttp-jinja2, as I suggested, resolves the path, but now the yarl/ coverage seems to have disappeared: #1028. |
@Dreamsorcerer there's a few points to check:
It is a good idea to bisect Codecov @ https://app.codecov.io/gh/aio-libs/yarl/commits. Looks like https://app.codecov.io/gh/aio-libs/yarl/commit/60737a8bf57625cffb09674b788ab2f64320a448 is the last commit with proper paths and the following https://app.codecov.io/gh/aio-libs/yarl/commit/e74c1599d47e21523d3e78ad78966d9630b12cc0 is broken. So it looks like #1007 might've influenced it, which could only come from the MyPy bump there or unpinned deps. Comparing the last working (https://api.codecov.io/upload/gh/aio-libs/yarl/download?path=v4/raw/2024-06-06/F2143C32CD85E0BC827E552A5E512211/60737a8bf57625cffb09674b788ab2f64320a448/2bd0dbd5-af59-44bd-8e2b-479f8d54ef00.txt) and the first broken (https://api.codecov.io/upload/gh/aio-libs/yarl/download?path=v4/raw/2024-06-06/F2143C32CD85E0BC827E552A5E512211/e74c1599d47e21523d3e78ad78966d9630b12cc0/6a695c6c-3879-48c0-bf3d-62e2d036817a/d9f3f16e-284b-43ce-a191-ce045b11982f.txt) uploads (both from Py-3.11.9 @ ubuntu-latest), the first thing I noticed is that the file list before the XML chunk that was present earlier, disappeared. The second thing is that the older working upload had a This points to some env changes, which might be versions of pytest/pytest-cov/codecov-action/codecov-cli etc. |
@Dreamsorcerer you could also try producing an html report locally to see if |
Oh... There's actually more commits in between 60737a8 and e74c159: https://github.com/aio-libs/yarl/commits/e74c1599d47e21523d3e78ad78966d9630b12cc0/. And among those, there's a series of action bumps, which probably didn't have a chance to make uploads, being cancelled by newer merges starting more CI jobs. I compared our two commits' logs and the confirmed my suspicion: - Download action repository 'codecov/[email protected]' (SHA:eaaf4bedf32dbdc6b720b63067d99c4d77d6047d)
+ Download action repository 'codecov/codecov-action@v4' (SHA:125fc84a9a348dbcf27191600683ec096ec9021c) (https://github.com/aio-libs/yarl/actions/runs/9401318235/job/25893141828#step:1:41 vs. https://github.com/aio-libs/yarl/actions/runs/9405561693/job/25907242226#step:1:41) With that in mind, my new suspect is... 🥁 🪘 🥁 ... #988! |
Good news is that if you're persistent enough, it's possible to get them to merge your bugfix: https://github.com/codecov/uploader/pulls?q=sort%3Aupdated-desc+is%3Apr+author%3Awebknjaz+is%3Amerged / https://github.com/codecov/codecov-cli/pulls?q=sort%3Aupdated-desc+is%3Apr+is%3Amerged+author%3Awebknjaz. (at least two of those are fixes for “innocent” refactoring, by the way, hence https://github.com/orgs/aio-libs/discussions/36) |
To conclude, the current workaround for viewing the coverage would be |
Tried v3.1.4 - https://github.com/aio-libs/yarl/actions/runs/9691011881/job/26741939301 |
I ended up hardcoding the token in pytest the other day (pytest-dev/pytest#12516), to fight this problem. I think, we should do it everywhere for now. |
By the way, another configuration problem is that passing the token was never implemented for the linters job: https://github.com/aio-libs/yarl/pull/988/files#r1631478435 — I haven't yet addressed that. |
Yes, I integrated putting that into the job summary directly from whatever coverage.py collected. There's no codecov integration at that point, so it doesn't stand in the way in that specific place. |
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
What do these changes do?
The changes honor empty segments when joining urls
e.g.
Picking up the PR #1023 to get this merged.
Are there changes in behavior for the user?
The changes to URL.join and URL.joinpath do not strip empty segments any longer.
Related issue number
Fixes #984
Fixes #926
Checklist